Skip to content

Conversation

@andriyDev
Copy link
Contributor

Objective

Solution

  • Stop using Path::join for joining asset paths and instead use AssetPath::resolve_embed.

Testing

  • Added 2 tests!

@andriyDev andriyDev added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-glTF Related to the glTF 3D scene/model format labels Oct 23, 2025
@andriyDev
Copy link
Contributor Author

As an aside, I think we should remove LoadContext::path and just stick to supporting LoadContext::asset_path. It seems like a footgun to provide the std::path::Path since they don't work with custom asset soruces. If a user who needs the raw Path, they can just do context.asset_path().path().

That's for a future PR though.

@alice-i-cecile alice-i-cecile added this to the 0.17.3 milestone Oct 23, 2025
@viridia
Copy link
Contributor

viridia commented Oct 23, 2025

Yes, please, kill it with fire!

@andriyDev andriyDev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 23, 2025
@mockersf mockersf added this pull request to the merge queue Oct 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 25, 2025
@andriyDev
Copy link
Contributor Author

andriyDev commented Oct 25, 2025

Wow that random flake actually caught another issue! Apparently materials are doing their own load of the textures, but this load always just fetches the handle from the previous texture loads (since we're still holding those handles).

I've now checked to make sure there are no other joins so we shouldn't have any more of these issues.

As an aside, I don't think there's a need for us to be doing a second load for the materials. We should just propagate the texture handles through to these functions so they can get the texture handles that way. Having the loads just happen to call the same path seems incredibly brittle. Another PR though haha.

Edit: I looked into why we need a second load for the materials. The problem is that we only update the dependencies of a subasset if there is a load involved. If we just passed in the texture handles, we wouldn't get that load, so our material wouldn't have the texture as a dependency, meaning the is_loaded_with_dependencies check would be wrong. We should probably just be calling VisitDependencies in the load_context to solve this at some point.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 29, 2025
Merged via the queue into bevyengine:main with commit f6b34bd Oct 29, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds A-glTF Related to the glTF 3D scene/model format C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom asset source does not work with GLTF

5 participants